Skip to content

Conversation

@robertsipka
Copy link
Contributor

No description provided.

@robertsipka robertsipka added the debugger Related to the debugger label Feb 19, 2019
@robertsipka robertsipka requested a review from zherczeg February 19, 2019 08:53
Copy link
Member

@zherczeg zherczeg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice patch! Only one minor comment fix.

return self.socket.send(data)

def ready(self):
""" Monitor the file descriptor, return weither it became ready for the I/O operations or not. """
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should only be used with receive_data, so the comment is not exactly right.

if self.socket not in result:
return False

return True
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can use return self.socket in result here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, good catch. :)

…unication protocols

JerryScript-DCO-1.0-Signed-off-by: Robert Sipka rsipka.uszeged@partner.samsung.com
@robertsipka robertsipka force-pushed the separate_communcation_layers branch from 5ec6ecb to d3e01be Compare February 20, 2019 13:03
@robertsipka
Copy link
Contributor Author

@zherczeg @rerobika : Thanks, I've modified the patch accordingly.

Copy link
Member

@rerobika rerobika left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (informal)

Copy link
Member

@zherczeg zherczeg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@LaszloLango LaszloLango left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@zherczeg zherczeg merged commit 6d490c7 into jerryscript-project:master Feb 26, 2019
WEBSOCKET_FIN_BIT = 0x80

class WebSocket(object):
def __init__(self, address, protocol=Socket()):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the Socket() as default argument is not correct. The protocol will have a reference to the created object and if the WebSocket is constructed multiple times the same socket will be used in both cases.
example.:

a = WebSocket()
b = WebSocket()
a.protocol == b.protocol

We should use None as default and add an check in the constructor to create the socket object.

self.data_buffer += self.protocol.receive_data()

if self.data_buffer[0:len_expected] != expected:
raise Exception("Unexpected handshake")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the future it would be good to have specialized Exception classes so we can catch our module's exceptions in a uniform fashion and would make it possible to distinguish between the "base" exceptions ans our exceptions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This applies to all exceptions in this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

debugger Related to the debugger

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants